Skip to content

Do not force cli to refresh the display#190

Merged
HenrikBengtsson merged 1 commit intofutureverse:developfrom
DavisVaughan:fix/cli-force
Jan 28, 2026
Merged

Do not force cli to refresh the display#190
HenrikBengtsson merged 1 commit intofutureverse:developfrom
DavisVaughan:fix/cli-force

Conversation

@DavisVaughan
Copy link
Contributor

@DavisVaughan DavisVaughan commented Jan 27, 2026 •
edited
Loading

Closes #167

Instead, rely on cli's own internal timing and trust that it knows when to refresh the display at regular intervals.

forceing a refresh results in WAY slower progress bars, because it forces cli to do a whole bunch of work at each iteration. In particular, it calls cli:::inline_transformer() via cli:::glue() WAY more often, which is what does the inline interpolation in cli. As shown in #167 (comment), this takes a huge amount of time.

Running the original example from #167

f <- function(R = 1000) {
  pb <- progressr::progressor(steps = R)
  onerun <- function(...) {
    pb()
    Sys.sleep(0.001)
  }
  system.time(lapply(seq_len(R), onerun))
}

progressr::handlers(global = TRUE)

progressr::handlers(progressr::handler_void)
f() |> print()
#>   user  system elapsed 
#>  0.155   0.011   1.427 

progressr::handlers(progressr::handler_progress)
f() |> print()
#>   user  system elapsed 
#>  0.401   0.019   1.672 

progressr::handlers(progressr::handler_cli)
f() |> print()
#>   user  system elapsed 
#>  0.234   0.010   1.498

Cranked up to 10k iterations

f <- function(R = 10000) {
  pb <- progressr::progressor(steps = R)
  onerun <- function(...) {
    pb()
    Sys.sleep(0.001)
  }
  system.time(lapply(seq_len(R), onerun))
}

progressr::handlers(global = TRUE)

progressr::handlers(progressr::handler_void)
f() |> print()
#>   user  system elapsed 
#>  2.017   0.138  14.910 

progressr::handlers(progressr::handler_progress)
f() |> print()
#>   user  system elapsed 
#>  4.663   0.424  17.605 

progressr::handlers(progressr::handler_cli)
f() |> print()
#>   user  system elapsed 
#>  3.081   0.383  16.065

@DavisVaughan
Do not force cli to refresh the display
8867012 
Instead, rely on cli's own internal timing and trust that it knows when to refresh the display.
@DavisVaughan DavisVaughan mentioned this pull request Jan 27, 2026
Draft
4 tasks
@HenrikBengtsson
Copy link
Collaborator

HenrikBengtsson commented Jan 28, 2026

Great catch. This was introduced in 01eb29a in order to support cli reporting progress via progressr, e.g.

library(cli)

options(cli.progress_handlers = "progressr")

clean <- function() {
  cli_progress_bar("Cleaning data", total = 100)
  for (i in 1:100) {
    Sys.sleep(5/100)
    cli_progress_update()
  }
  cli_progress_done()
}

clean()

I just checked, and it looks like it still works without force = TRUE. I don't recall why I added that in that commit - maybe a "just-in-case" addition.

@HenrikBengtsson HenrikBengtsson merged commit 5365a63 into futureverse:develop Jan 28, 2026
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Slow performance with progressr::handler_cli

2 participants

@DavisVaughan @HenrikBengtsson